inference: Fix correctness and ensure termination in the presence of PhiNodes #53876
Conversation
|
I don't think SSAValue was intended to contribute anything towards convergence, so doing so just slightly slows inference and maybe worsens the quality. But PHINodes certainly would need to be implemented to handle convergence just like a Slot. |
vtjnash
added
needs nanosoldier run
|
Ok, I can move this handling to PhiNode, which is probably the proper place for it. It's possible similar handling will be required for some of the exception propagation stuff, but let's cross that particular bridge if it comes up. |
…PhiNodes There's two related, but distinct, issues here: 1. We were not using `tmerge` for merging SSA results inside loops, which could cause infinite looping. In the absence of PhiNodes, things usually have to go through a slot to be able to make the round trip, which would usually put a PhiNode on the path, but it's possible there may be other ways to smuggle things around (e.g. through exception handling). 2. We were not properly accounting for the fact that PhiNode uses do not need to be linearly ordered in the same BB, so we were getting the type of the testcase here incorrect by failing to re-schedule the PhiNode. The first of these shows up in the Diffractor test suite, the second was found by writing the test case.
|
@nanosoldier |
|
Yes, I think this seems right |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
@nanosoldier runbenchmarks("inference", vs=":master") I don't entirely trust that result. Let me make sure it reproduces. |
|
@nanosoldier |
|
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
There's two related, but distinct, issues here:
We were not using
tmergefor merging SSA results inside loops, which could cause infinite looping. In the absence of PhiNodes, things usually have to go through a slot to be able to make the round trip, which would usually put a PhiNode on the path, but it's possible there may be other ways to smuggle things around (e.g. through exception handling).We were not properly accounting for the fact that PhiNode uses do not need to be linearly ordered in the same BB, so we were getting the type of the testcase here incorrect by failing to re-schedule the PhiNode.
The first of these shows up in the Diffractor test suite, the second was found by writing the test case.